Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to set size limit for slug #809

Merged
merged 3 commits into from
Aug 7, 2017

Conversation

avokhmin
Copy link
Contributor

.travis.yml Outdated
- 2.3.0
- 2.2.0
- 2.1.0
- 2.0.0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we can do it, because Support of Ruby 2.1 has ended

.travis.yml Outdated
- rvm: 2.4.0
gemfile: gemfiles/Gemfile.rails-4.0.rb
- rvm: 2.4.0
gemfile: gemfiles/Gemfile.rails-4.1.rb
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://travis-ci.org/Shuttlerock/friendly_id/jobs/232872362

TypeError: Cannot visit Integer
    /home/travis/build/Shuttlerock/friendly_id/gemfiles/vendor/bundle/ruby/2.4.0/gems/arel-5.0.1.20140414130214/lib/arel/visitors/visitor.rb:28:in `rescue in visit'

http://weblog.rubyonrails.org/2017/3/20/Rails-5-1-rc1/

As per our maintenance policy, the release of Rails 5.1 will mean that bug fixes will only apply to 5.1.x, regular security issues to 5.1.x and 5.0.x, and severe security issues to 5.1.x, 5.0.x, and 4.2.x. This means 4.x and below will essentially be unsupported!

I think that we can skip 4.0 and 4.1 here too.

@avokhmin
Copy link
Contributor Author

@norman , can you write a feedback?

@norman
Copy link
Owner

norman commented Jul 13, 2017

Thanks for your time and efforts, @avokhmin! I'm not currently actively maintaining FriendlyId any more - @parndt is the main maintainer now, but I do have some feedback.

First, the changes to the CI setup look great, but it would probably be better to have those done in a separate PR from this in order to keep things focused. I suspect those changes could be merged right away, whereas the other stuff might need some more time and discussion. Thank you for that! Keeping the CI setup working properly is useful and valuable work.

Regarding the slug limit, this is a feature that used to be in FriendlyId 3.0, but that I removed from FriendlyId 4.0. My idea at the time was that the size limit could be controlled with Active Record validations or column size limits, so it didn't need to be in the library. Version 3.0 had grown to be so large that I wanted to remove as much as possible from the library to keep it easier to maintain and less likely to conflict with other libraries.

The natural lifecycle of plugins like this is to add, add, add and add more and more and more features; everybody has a good reason for wanting to add the thing they want, and it might be legitimately useful and be well written (as your code looks to be here). But if we accept every one that seems justifiable and well written, eventually the library becomes "bloated". Then, somebody else comes along and writes a competing library that's "like FriendlyId but with less bloat", repeat the cycle over and over again. I think we've all seen this. This is what I wanted to avoid in FriendlyId 4.0 and so I rewrote it from the ground up to be simpler and smaller, and less likely to be a library that projects would want to remove because of bloat. It may sound counterintuitive but I've always encouraged people to use FriendlyId for as little as possible, which I think makes it more useful over the long term.

Your feature looks legitimately useful and well coded, so I'll leave it up to @parndt and other folks if they want to include it, but that's the background on why this feature is not currently included in the library.

@parndt
Copy link
Collaborator

parndt commented Jul 15, 2017

Thank you @norman

@avokhmin I agree with @norman here, can you introduce 9ff5fa6 as its own PR so that it's easier to reason about this PR?

Let me know if you would like pointers about that and I'll do my best to help.

@avokhmin
Copy link
Contributor Author

avokhmin commented Aug 1, 2017

@avokhmin I agree with @norman here, can you introduce 9ff5fa6 as its own PR so that it's easier to reason about this PR?

@parndt done, see: #824

@avokhmin
Copy link
Contributor Author

avokhmin commented Aug 2, 2017

git rebase master

@parndt, PR updated.

max_candidate_size = [max_candidate_size, 0].max
candidate = candidate[0...max_candidate_size]
end
[candidate, uid].compact.join(friendly_id_config.sequence_separator)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK just one last comment. I would use the 'extract method' refactor here:

def resolve_friendly_id_conflict(candidates)
  uuid = SecureRandom.uuid
  [
    apply_slug_limit(candidates.first, uuid), 
    uuid
  ].compact.join(friendly_id_config.sequence_separator)
end

def apply_slug_limit(candidate, uuid)
  return candidate unless candidate && friendly_id_config.slug_limit
  
  candidate[0...slug_limit(uuid)]
end
private :apply_slug_limit

def slug_limit(uuid)
  [
    friendly_id_config.slug_limit - uuid.size - friendly_id_config.sequence_separator.size,
    0
  ].max
end
private :max_candidate_size_for_uuid

I haven't tried the code, but would something along those lines work?
My rationale is that it just makes the original method easier to reason about rather than having to read the code a few times to figure out what's going on with the conditional. It also means others can use this functionality without rewriting it if they override resolve_friendly_id_conflict. 😄

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may even be a simpler version waiting to be found, but I wanted to communicate my thoughts with Ruby code. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it, looks much better 👍

@avokhmin
Copy link
Contributor Author

avokhmin commented Aug 3, 2017

@parndt, PR updated.

@avokhmin
Copy link
Contributor Author

avokhmin commented Aug 7, 2017

@parndt , something wrong here?

@parndt
Copy link
Collaborator

parndt commented Aug 7, 2017

No, just a lack of time at the computer 😄 thanks a lot @avokhmin

@parndt parndt merged commit e873122 into norman:master Aug 7, 2017
@avokhmin
Copy link
Contributor Author

avokhmin commented Sep 5, 2017

@parndt , can I create a PR for next release? Or when it can be?

@parndt
Copy link
Collaborator

parndt commented Sep 11, 2017

@avokhmin please, create a PR 😄 👍 thanks

@avokhmin
Copy link
Contributor Author

@parndt , done, see: #828

@nuzelac
Copy link

nuzelac commented Jan 28, 2018

@avokhmin

Does setting slug_limit in the configuration work for you?
I'm using Rails 5.0.6 and friendly_id 5.2.3. I can use :slug_limit from the model:

friendly_id :name, use: :slugged, slug_limit: 50

But if I move it into the initializer:

#
# By default, slug has no size limit, but you can change it if you wish.
#
config.slug_limit = 50

I receive this error after running Product.find_each(&:save):

NoMethodError: undefined method `slug_limit=' for #<#<Class:0x007fd3ea4cf4a8>:0x007fd3ea4cf458>

@avokhmin
Copy link
Contributor Author

@nuzelac , I use it only in the config/initializers/friendly_id.rb:

FriendlyId.defaults do |config|
  config.use :slugged, :reserved
  config.slug_limit = 255
end

Gemfile.lock

    friendly_id (5.2.2)
    rails (5.1.4)

do you use :slugged ?

@nuzelac
Copy link

nuzelac commented Jan 30, 2018

Thank you @avokhmin. I added config.use :slugged and now it works from the initializer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants